Support Interactive without setting verbosity explicitly in dotnet tool restore Fixes #22987 #45312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-Tools
untriaged
This was referenced Dec 20, 2024
|
I had a lot of difficulty testing this. Special thanks to @nkolev92 for suggesting an authenticated feed I could use for testing, which ended up working beautifully once I figured out a way to get rid of my tokens. Screenshots in action: With --interactive (but not verbosity): |
baronfel
reviewed
Dec 20, 2024
Comment on lines
+66
to
+74
| if (!result.HasOption(ToolRestoreCommandParser.VerbosityOption) && result.GetValue(ToolCommandRestorePassThroughOptions.InteractiveRestoreOption)) | ||
| { | ||
| _verbosity = VerbosityOptions.minimal; | ||
| } | ||
|
|
||
| _restoreActionConfig = new RestoreActionConfig(DisableParallel: result.GetValue(ToolCommandRestorePassThroughOptions.DisableParallelOption), | ||
| NoCache: result.GetValue(ToolCommandRestorePassThroughOptions.NoCacheOption) || result.GetValue(ToolCommandRestorePassThroughOptions.NoHttpCacheOption), | ||
| IgnoreFailedSources: result.GetValue(ToolCommandRestorePassThroughOptions.IgnoreFailedSourcesOption), | ||
| Interactive: result.GetValue(ToolCommandRestorePassThroughOptions.InteractiveRestoreOption)); |
There was a problem hiding this comment.
This is great! I have two broad comments:
- I feel like I've seen this 'verbosity needs to be downstream of some other configuration' problem in a few different places in the codebase - is it something that you think we could detect and extract to prevent playing whack-a-mole?
- the set of parameters to the RestoreActionConfig seem very 'regular' (in the sense of common/not-changing) - is there a way we could enforce that
- commands that use RestoreActionConfig have the required options applied to them and
- something in the way those commands are processed/executed automatically configures a RestoreActionConfig? Again focusing on 'How can we be consistent by design' here instead of playing whack-a-mole. Maybe a middleware or extension method on ParseResult or something?
baronfel
approved these changes
Dec 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #22987
We don't currently pass a RestoreActionConfig from ToolRestoreCommand to the ToolPackageDownloader (and by extension the NuGetPackageDownloader) we actually use to install the tool. This corrects that error.
Additionally, the default verbosity is quiet. When the interactive flag is passed, that effectively suppresses it. This increases the default verbosity from quiet to minimal when interactive is requested.